Go: Use shared guards library by owen-mc · Pull Request #22365 · github/codeql

owen-mc · 2026-08-17T16:31:01Z

Instantiate the shared guards library for Go, port over all uses of the Go-specific ConditionGuardNode and deprecate it.

Go-specific issues encountered:

functions with multiple return values.
that the syntactic receiver and the semantic receiver of a method call may be different, e.g. in case of a promoted method or an implicit dereference.
functions with named result parameters may implicitly return values, rather than explicitly as part of a return statement.

github-advanced-security

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

owen-mc · 2026-10-06T16:28:09Z

"Run QL for QL" is failing for all PRs at the moment. I don't think it is related to this PR.

Copilot

Copilot review overview

🟡 Changes recommended

The deprecation change note incorrectly states that the legacy class has no instances.

Review effort: Balanced
Findings: 1

Open (1)

Correct inaccurate claim that deprecated API has no instances · New

What changed in this PR

Migrates Go guard reasoning to the shared control-flow guards library and updates dependent analyses, models, and tests.

Changes:

Adds the Go Guard/GuardValue implementation and shared-library extension points.
Migrates guard consumers and barrier models to the new APIs.
Deprecates the legacy guard API and expands regression coverage.

File	Description
`shared/controlflow/codeql/controlflow/Guards.qll`	Extends shared guard capabilities.
`java/ql/lib/semmle/code/java/controlflow/Guards.qll`	Adapts Java to the extended interface.
`go/ql/test/query-tests/Security/CWE-295/DisabledCertificateCheck/main.go`	Adds switch-based flag coverage.
`go/ql/test/query-tests/Security/CWE-209/test.go`	Adds switch-flow coverage.
`go/ql/test/query-tests/Security/CWE-209/StackTraceExposure.expected`	Updates expected findings.
`go/ql/test/library-tests/semmle/go/dataflow/GuardingFunctions/test.go`	Expands wrapper-guard tests.
`go/ql/test/library-tests/semmle/go/controlflow/Guards/Guards.ql`	Adds guard API test queries.
`go/ql/test/library-tests/semmle/go/controlflow/Guards/guards.go`	Adds comprehensive guard fixtures.
`go/ql/test/library-tests/semmle/go/controlflow/Guards/Guards.ext.yml`	Adds barrier-guard models.
`go/ql/test/library-tests/semmle/go/controlflow/Guards/Guards.expected`	Records expected guard results.
`go/ql/test/experimental/CWE-942/CorsMisconfiguration.go`	Adds switch-based CORS coverage.
`go/ql/test/experimental/CWE-942/CorsMisconfiguration.expected`	Updates expected CORS findings.
`go/ql/test/experimental/CWE-942/CONSISTENCY/DataFlowConsistency.expected`	Updates consistency expectations.
`go/ql/src/Security/CWE-327/InsecureTLS.ql`	Uses shared flag-control reasoning.
`go/ql/src/Security/CWE-295/DisabledCertificateCheck.ql`	Uses shared flag-control reasoning.
`go/ql/src/Security/CWE-209/StackTraceExposure.ql`	Migrates debug-flag barriers.
`go/ql/src/Security/CWE-020/IncompleteHostnameRegexp.ql`	Migrates regexp guard reasoning.
`go/ql/src/InconsistentCode/LengthComparisonOffByOne.ql`	Migrates comparison guards.
`go/ql/src/InconsistentCode/ConstantLengthComparison.ql`	Migrates length guards.
`go/ql/src/experimental/IntegerOverflow/RangeAnalysis.qll`	Uses shared range guards.
`go/ql/src/experimental/CWE-942/CorsMisconfiguration.ql`	Migrates CORS guard logic.
`go/ql/src/experimental/CWE-807/SensitiveConditionBypass.ql`	Migrates sensitive-condition guards.
`go/ql/lib/semmle/go/security/InsecureFeatureFlag.qll`	Adds flag-control abstraction.
`go/ql/lib/semmle/go/dataflow/internal/DataFlowUtil.qll`	Migrates barrier-guard infrastructure.
`go/ql/lib/semmle/go/dataflow/internal/DataFlowPrivate.qll`	Migrates unreachable-region guards.
`go/ql/lib/semmle/go/dataflow/ExternalFlow.qll`	Supports all modeled guard values.
`go/ql/lib/semmle/go/controlflow/IR.qll`	Clarifies conditional IR handling.
`go/ql/lib/semmle/go/controlflow/Guards.qll`	Instantiates shared guards for Go.
`go/ql/lib/semmle/go/controlflow/ControlFlowGraph.qll`	Deprecates legacy guard APIs.
`go/ql/lib/change-notes/2026-08-17-shared-guards.md`	Announces the shared guard API.
`go/ql/lib/change-notes/2026-08-17-deprecate-condition-guard-node.md`	Announces legacy API deprecation.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

owen-mc · 2026-10-10T17:08:00Z

There seems to be a performance problem which is causing some DCA jobs to time out. I'll investigate on Tuesday.

Add the complete guard regression suite and fold the shared hooks and Go-specific correctness fixes into the initial adapter. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Move simple data-flow and security consumers to shared guards, including value-aware barriers and the corrected feature-flag semantics. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

owen-mc · 2026-10-14T13:05:12Z

@aschackmull This is now ready to review.

I've resolved the big performance issues. The latest DCA run now shows and average 5% increase in analysis time. I investigated this in a few cases and wasn't able to easily reduce it - I think it is acceptable because we are now doing a more thorough analysis that we used to, e.g. conditions being propagated through variables and phi nodes.

The test failures seem spurious.

aschackmull · 2026-10-19T07:44:23Z

+
+  class Expr extends G::Expr {
+    /** Gets the associated control flow node. */
+    CfgImpl::Cfg::ControlFlowNode getControlFlowNode() { result = IR::evalExprInstruction(this) }


This is intended to be result.injects(this). AFAICS the only difference is for defers, is that a relevant distinction or could we use injects here?

aschackmull · 2026-10-19T07:51:24Z

+   * This supports implicit equality tests that do not have a corresponding source expression,
+   * such as comparisons between a switch expression and a case expression.


This should already work without an additional input predicate.

aschackmull · 2026-10-19T08:06:15Z

+    }
+  }
+
+  predicate caseOutcomeBlock(Case guard, CfgImpl::Cfg::BasicBlock outcomeBlock, boolean branch) {


What's the purpose of this signature extension? Could you elaborate on what didn't work without it? It seems quite narrow as it's restricted to default cases, so I wonder what hole it's trying to plug.

aschackmull · 2026-10-19T08:12:25Z

+  /**
+   * Holds if `outcomeBlock` contains a control flow node indicating that
+   * `guard` evaluated to `branch`.
+   */
+  default predicate booleanOutcomeBlock(Expr guard, BasicBlock outcomeBlock, boolean branch) {
+    none()
+  }


I'm aware that the Guards library likely needs an extension for covering Guards with after-value nodes that are fully determined by prior branches. I.e. a disjunction in a test position like if (a || b) is currently not working as a guard - only the individual a and b are guards here. Is it correctly understood that it is this particular corner-case that you're trying to handle here? I'd much prefer if such an extension was the subject of its own PR instead. Really, this ties into a potentially larger Guards refactor to better take into account the structure of the new CFG.

aschackmull · 2026-10-19T08:17:35Z

+     * Holds if `def` defines the implicit return value represented by `method`, or is an SSA
+     * definition traversed while determining that value.
+     */
+    default predicate relevantImplicitReturnSsaDefinition(
+      NonOverridableMethod method, SsaDefinition def
+    ) {
+      any()


Whoah! The default is a huge cartesian product?

aschackmull · 2026-10-19T08:26:22Z

+private module LogicInput implements GuardsImpl::LogicInputSig {
+  final private class FinalSsaDefinition = GoSsa::SsaDefinition;
+
+  class SsaDefinition extends FinalSsaDefinition {


Note: If Go used the MakeSsa module exposed by the shared SSA lib to build its SSA classes then the result should be directly pluggable with a one-line import here. (Just something to consider for potential followup).

aschackmull · 2026-10-19T08:31:22Z

+     * This supports languages where a return statement can return a named result
+     * without containing a return expression.
+     */
+    default predicate implicitReturnDefinition(NonOverridableMethod method, SsaDefinition def) {


This seems very Go-specific. I wonder whether we could instead just include any expressions being written to def as the return expressions instead? That way we'd stay within the existing signature and not need this extension.

github-actions Bot added documentation Go labels Aug 17, 2026

github-advanced-security Bot found potential problems Aug 17, 2026

View reviewed changes

Comment thread go/ql/lib/semmle/go/controlflow/Guards.qll Fixed

Comment thread go/ql/lib/semmle/go/controlflow/Guards.qll Fixed

Comment thread go/ql/lib/semmle/go/dataflow/internal/DataFlowUtil.qll Fixed

owen-mc force-pushed the go/shared-guards branch from f8092fc to 57db4c8 Compare August 17, 2026 19:16

github-advanced-security Bot found potential problems Aug 17, 2026

View reviewed changes

owen-mc force-pushed the go/shared-guards branch 2 times, most recently from def5d61 to 888eae3 Compare August 18, 2026 11:45

owen-mc force-pushed the go/shared-guards branch from 888eae3 to 82e42b4 Compare September 20, 2026 22:01

github-advanced-security Bot found potential problems Sep 20, 2026

View reviewed changes

Comment thread go/ql/lib/semmle/go/controlflow/Guards.qll Fixed

owen-mc mentioned this pull request Sep 22, 2026

Go: un-deprecate ParenExpr #22650

Merged

owen-mc force-pushed the go/shared-guards branch from c60e983 to 45fd28e Compare September 22, 2026 15:40

owen-mc force-pushed the go/shared-guards branch from 17bf057 to e3e1dcb Compare September 30, 2026 22:46

github-advanced-security Bot found potential problems Sep 30, 2026

View reviewed changes

owen-mc force-pushed the go/shared-guards branch from e3e1dcb to 05e44c8 Compare October 1, 2026 13:53

github-actions Bot added the Java label Oct 1, 2026

owen-mc force-pushed the go/shared-guards branch from 05e44c8 to bf97136 Compare October 6, 2026 14:46

github-advanced-security Bot found potential problems Oct 6, 2026

View reviewed changes

Comment thread go/ql/lib/semmle/go/controlflow/Guards.qll Fixed

owen-mc force-pushed the go/shared-guards branch from bf97136 to 97e898b Compare October 6, 2026 15:42

owen-mc mentioned this pull request Oct 6, 2026

QL-for-QL: fix parsing of overlay[local?] and overlay[caller?] annotations #22763

Merged

owen-mc marked this pull request as ready for review October 6, 2026 16:28

Copilot AI balanced review requested due to automatic review settings October 6, 2026 16:28

owen-mc requested review from a team as code owners October 6, 2026 16:28

Copilot started reviewing on behalf of owen-mc October 6, 2026 16:28

View session

Copilot AI reviewed Oct 6, 2026

View reviewed changes

Comment thread go/ql/lib/change-notes/2026-08-17-deprecate-condition-guard-node.md Outdated

owen-mc requested a review from aschackmull October 9, 2026 15:51

owen-mc force-pushed the go/shared-guards branch from fc8d706 to c05aecb Compare October 9, 2026 15:52

owen-mc force-pushed the go/shared-guards branch 3 times, most recently from c4929fe to dfff37f Compare October 13, 2026 19:20

owen-mc and others added 7 commits October 13, 2026 23:45

Go: instantiate the shared guards library

0c0ebf9

Add the complete guard regression suite and fold the shared hooks and Go-specific correctness fixes into the initial adapter. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Go: migrate simple guard consumers

8007acf

Move simple data-flow and security consumers to shared guards, including value-aware barriers and the corrected feature-flag semantics. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Go: migrate remaining guard consumers

41c9afd

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Go: deprecate ConditionGuardNode

d5a6f9e

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Document Go shared guard migration

9ca7e14

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Allow functions with multiple return values in Guards logic

be72021

Refactor: add Expr.getControlFlowNode and Expr.getBasicBlock

32d4595

owen-mc force-pushed the go/shared-guards branch from dfff37f to 32d4595 Compare October 14, 2026 08:37

aschackmull reviewed Oct 19, 2026

View reviewed changes

		* This supports implicit equality tests that do not have a corresponding source expression,
		* such as comparisons between a switch expression and a case expression.

Conversation

owen-mc commented Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-advanced-security AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

owen-mc commented Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

owen-mc commented Oct 10, 2026

Uh oh!

owen-mc commented Oct 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aschackmull Oct 19, 2026

Choose a reason for hiding this comment

Uh oh!

aschackmull Oct 19, 2026

Choose a reason for hiding this comment

Uh oh!

aschackmull Oct 19, 2026

Choose a reason for hiding this comment

Uh oh!

aschackmull Oct 19, 2026

Choose a reason for hiding this comment

Uh oh!

aschackmull Oct 19, 2026

Choose a reason for hiding this comment

Uh oh!

aschackmull Oct 19, 2026

Choose a reason for hiding this comment

Uh oh!

aschackmull Oct 19, 2026

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

owen-mc commented Aug 17, 2026 •

edited

Loading

owen-mc commented Oct 6, 2026 •

edited

Loading

owen-mc commented Oct 14, 2026 •

edited

Loading